Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support showing NSX LB SNAT IP in networkinfo CR #1018

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timdengyun
Copy link
Contributor

@timdengyun timdengyun commented Jan 27, 2025

Getting VPC policy Tier1 uplink port IP as NSX LB SNAT IP in order to present it in networkinfo CR loadBalancerIPAddresses field.

Test done:
NSX API output for vpc-vw8t for VPC T1 uplink and downlink Port info

GET https://10.192.27.76/policy/api/v1/infra/realized-state/realized-entities?intent_path=/orgs/default/projects/a9425f85-3da3-462d-9957-c44d729d7e63/vpcs/vpc-vw8t
{
            "extended_attributes": [
                {
                    "data_type": "STRING",
                    "multivalue": true,
                    "values": [
                        "100.64.0.2/31"
                    ],
                    "key": "IpAddresses"
                },
                {
                    "data_type": "STRING",
                    "multivalue": false,
                    "values": [
                        "02:50:56:56:44:52"
                    ],
                    "key": "MacAddress"
                }
            ],
            "entity_type": "RealizedLogicalRouterPort",
            "intent_paths": [
                "/orgs/default/projects/a9425f85-3da3-462d-9957-c44d729d7e63/vpcs/vpc-vw8t",
                "/orgs/default/projects/a9425f85-3da3-462d-9957-c44d729d7e63/transit-gateways/default"
            ],
            "resource_type": "GenericPolicyRealizedResource",
            "id": "vpc-vw8t",
            "display_name": "default-vpc-vw8t-t0_lrp",
            "path": "/orgs/default/projects/a9425f85-3da3-462d-9957-c44d729d7e63/realized-state/enforcement-points/default/transit-gateways/default/vpc-interfaces/vpc-vw8t",
            "relative_path": "vpc-vw8t",
           ..
        },
{
            "extended_attributes": [
                {
                    "data_type": "STRING",
                    "multivalue": true,
                    "values": [
                        "100.64.0.3/31"
                    ],
                    "key": "IpAddresses"
                },
                {
                    "data_type": "STRING",
                    "multivalue": false,
                    "values": [
                        "02:50:56:56:44:55"
                    ],
                    "key": "MacAddress"
                }
            ],
            "entity_type": "RealizedLogicalRouterPort",
            "intent_paths": [
                "/orgs/default/projects/a9425f85-3da3-462d-9957-c44d729d7e63/vpcs/vpc-vw8t"
            ],
            "resource_type": "GenericPolicyRealizedResource",
            "id": "gateway-interface",
            "display_name": "default-vpc-vw8t-t1_lrp",
            "path": "/orgs/default/projects/a9425f85-3da3-462d-9957-c44d729d7e63/realized-state/enforcement-points/default/vpcs/vpc-vw8t/gateway-interface",
            "relative_path": "gateway-interface",
                 ..
        },

This PR is to show 100.64.0.3 as loadBalancerIPAddresses SNAT IP for namespace: ns-1 which is associated with VPC vpc-vw8t

kubectl get networkinfo ns-1 -nns-1 -o yaml
apiVersion: crd.nsx.vmware.com/v1alpha1
kind: NetworkInfo
metadata:
  creationTimestamp: "2025-01-21T17:16:23Z"
  generation: 3
  name: ns-1
  namespace: ns-1
  resourceVersion: "5848475"
  uid: 6bb633fc-08c8-4f95-9ca2-829033b6c2b2
vpcs:
- defaultSNATIP: 10.1.0.1
  loadBalancerIPAddresses: 100.64.0.3
  name: vpc-vw8t
  privateIPs:
  - 192.168.0.0/16

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 87.27273% with 7 lines in your changes missing coverage. Please review.

Project coverage is 74.30%. Comparing base (6c54354) to head (bbb2e85).

Files with missing lines Patch % Lines
.../controllers/networkinfo/networkinfo_controller.go 81.81% 4 Missing ⚠️
pkg/nsx/services/vpc/vpc.go 76.92% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1018      +/-   ##
==========================================
+ Coverage   74.24%   74.30%   +0.05%     
==========================================
  Files         118      118              
  Lines       16398    16445      +47     
==========================================
+ Hits        12175    12219      +44     
- Misses       3453     3455       +2     
- Partials      770      771       +1     
Flag Coverage Δ
unit-tests 74.30% <87.27%> (+0.05%) ⬆️
Files with missing lines Coverage Δ
pkg/nsx/services/realizestate/realize_state.go 100.00% <100.00%> (ø)
pkg/nsx/services/vpc/vpc.go 55.37% <76.92%> (+0.28%) ⬆️
.../controllers/networkinfo/networkinfo_controller.go 69.05% <81.81%> (+0.94%) ⬆️

Getting VPC Ppolicy Tier1 uplink port IP as NSX LB SNAT IP in order to
present it in networkinfo CR loadBalancerIPAddresses field.
@@ -679,6 +678,22 @@ func (s *VPCService) GetAVISubnetInfo(vpc model.Vpc) (string, string, error) {
return path, cidr, nil
}

func (s *VPCService) GetNSXLBSNATIP(vpc model.Vpc) (string, error) {
log.V(2).Info("Get VPC NSX LB SNAT IP", "VPC", *vpc.Id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get -> Getting? seem the convention uses "Getting" before operation and "Got" after operation finished.

Copy link
Contributor

@TaoZou1 TaoZou1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In
Getting VPC Ppolicy -> policy

for i := range extendAttributes {
if extendAttributes[i].Key != nil && *extendAttributes[i].Key == "IpAddresses" {
if len(result.IntentPaths) == 1 {
for _, ip := range extendAttributes[i].Values {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you move
if len(result.IntentPaths) == 1 before Line 91?
if if len(result.IntentPaths) != 1 {
continue
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If extendAttributes[i].Values has multiple values, it seems you will use the first legal one.
is it possible to have multiple legal value?

for _, ip := range extendAttributes[i].Values {
parts := strings.Split(ip, "/")
if len(parts) != 2 {
log.Info("Invalid tier1 uplink port IP found", "IP", ip, "Path", intentPath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log.Error?

return "", err
}

for _, result := range results.Results {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we filter by entity_type=RealizedLogicalRouterPort?

setNSNetworkReadyCondition(ctx, r.Client, req.Namespace, nsMsgVPCAviSubnetError.getNSNetworkCondition(err))
return common.ResultRequeueAfter10sec, err
}
lbIP = aviSECIDR
} else if lbProvider == vpc.NSXLB {
nsxLBSNATIP, err = r.Service.GetNSXLBSNATIP(*createdVpc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we return the VPC Tier1 LB SNAT IP in CreateOrUpdateVPC func when checking the realization state of VPC?
we can avoid a new NSX API call to get VPC realization state again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you also help check if VPC is created with AVI Subnet, we still can get the LB SNAT IP right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants